Since this is a connected component, let's use the recordGoogleEvent redux action . This makes it a lot easier to write any component tests.

So:

analytics.ga.recordEvent( 'Upgrades', `Clicked ${ this.props.eventFormName } State Select` );

turns into

const { recordGoogleEvent } = this.props;
recordGoogleEvent( 'Upgrades', `Clicked ${ this.props.eventFormName } State Select` );

Usage seems a bit awkward. Maybe let's update QueryCountryStates to do a no-op if countryCode is falsey?

Let's have the QueryComponent inline instead.

As of #6539, we encourage importing from the root Lodash module directly:

import { isEmpty } from 'lodash';

You're not dispatching an action here, only calling an action creator. The action creator will return a thunk, but you need to use react-redux's connect mapDispatchToProps to pass the action creator as a wrapped dispatch function prop into the component.

Oh sorry, forgot to add example connect code for that.

No reason to apologize, I should know better. I actually passed it to connect() at one point but for some reason decided to remove it again

We've enabled the react/jsx-no-bind ESLint rule which this violates because bind will always create a new function, thereby preventing any possible benefits of pure render of a child component. Instead, you should bind components in the constructor:

constructor() {
    super( ...arguments );

    this.recordStateSelectClick = this.recordStateSelectClick.bind( this );
}

recordStateSelectClick() {
    const { eventForm } = this.props;
    // ...
}

... or stage 1 class instance fields:

recordStateSelectClick = () => {
    const { eventForm } = this.props;
    // ...
}

My implementation currently looks like this:

recordStateSelectClick() {
    const { eventFormName, recordGoogleEvent } = this.props;

    if ( eventFormName ) {
        recordGoogleEvent( 'Upgrades', `Clicked ${ eventFormName } State Select` );
    }
}

This however gives me a no-shadow es-lint warning since I'm already importing recordGoogleEvent in the upper scope. Is there a better way to mitigate that than using this.props.recordGoogleEvent()?

I'm generally lazy and do this.props.recordGoogleEvent() but there's nothing stopping us from making another variable.

const { eventFromName, recordGoogleEvent: recordEvent } = this.props;
    if ( eventFormName ) {
        recordEvent( 'Upgrades', `Clicked ${ eventFormName } State Select` );
    }

Ha! I tried const { eventFromName, recordGoogleEvent as recordEvent } = this.props;. Learned something yet again :)

Yea it can be confusing since the import syntax is similar but not quite the same (i.e. import { recordGoogleEvent as recordEvent } from ''; would be correct).

Yes, that's what I thought of :)

We could probably avoid the htmlFor (which I don't think works anyways, since this assumes an ID on the <select>) by including the <FormSelect /> render as a child of the <FormLabel />.

It seems easier to add the id. Nesting it would require style updates since the bottom margin of the label element doesn't work with the select anymore.

It seems easier to add the id.

Not super easy in React, since multiple instances of the component could be present on the page, and the ID needs to be unique. So at a minimum it becomes something like:

class MyComponent extends Component {
    static instances = 0;

    componentWillMount() {
        this.instance = ++this.constructor.instances;
    }

    render() {
        return <input id={ this.instance } />
    }
}

Oy

It looks like it is assumed that there will ever be only one instance of each form field: https://github.com/Automattic/wp-calypso/blob/master/client/my-sites/upgrades/components/form/input.jsx#L81

Well, in that case the name would need to be a required prop, otherwise the htmlFor will be ineffective if omitted. Seems like a nuisance from an implementer's perspective. I personally think we should go the nesting route even if it requires style updates.

Real-world example of unique IDs: https://github.com/Automattic/wp-calypso/blob/master/client/components/spinner/index.jsx

Don't forget that we can use fancy es6/es2015 features because we use babel. In this case let's use export over module.exports

export default connect(
    ( state, { countryCode } ) => {
         return countryStates: countryCode ? getCountryStates( state, countryCode ) : []
    },
    { recordGoogleEvent } // this can be an object 
)( localize( StateSelect ) );

see also https://github.com/reactjs/react-redux/blob/master/docs/api.md#connectmapstatetoprops-mapdispatchtoprops-mergeprops-options